-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure ssrRewriteStacktrace doesn't throw #10638
fix: ensure ssrRewriteStacktrace doesn't throw #10638
Conversation
🦋 Changeset detectedLatest commit: afaa26d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: Willow (GHOST) <ghostdevbusiness@gmail.com>
This looks like a good change to me, but I'd like to get some more details. How do we reproduce this issue? Any idea why |
Unfortunately it's a super weird issue that I was not able to reproduce consistently...i only seen it happen in webcontainers but I while I was implementing an hacky fix for sveltelab (SvelteLab/SvelteLab#278) I discovered that locally it doesn't happen and it only happens on the deployed version on vercel. I was able to reproduce in stackblitz too tho. |
How often does it happen? Can you reproduce it somewhat consistently or is it very rare? I wonder if we should log that an error was encountered rather than silently ignoring it? On the one hand, I don't want to annoy users, but on the other, I don't want to just ignore that something bad is happening and cover up the underlying bug rather than fixing it @gtm-nayan pointed out the other day that Vite added a check to avoid rewriting stack traces twice in vitejs/vite@feb8ce0. As a result, we may not need |
I've also opened an issue on webcontainers with the reproduction. But basically either go on sveltelab or stackblitz and throw an error in the load function. Here's the issue on webcontainers ( stackblitz/webcontainer-core#1155 ) |
Maybe the best course of action would be still wrapping the call in a try catch given that it could throw but add the error to the returned stack in the catch? |
@paoloricciuti does the error still happen in the latest release since #10769 was merged? If so, this PR will need a rebase |
Uh I missed the linked PR...I have to check, I'll do it in a moment |
It seems to work fine on stackblitz ( I can't test on sveltelab because I was temporarily patching sveltekit with this solution to make it work) |
You don't need this change now, right? |
No it seems to have been fixed by #10769 |
This is a weird edge case so feel free to just close this PR if it doesn't make sense.
Basically right now there's a weird bug in webcontainers where any error during ssr makes the dev server crash. The reason it happens is because
ssrRewriteStackTrace
it's called twice (for some reason it doesn't happen locally).This PR fixes this behavior by wrapping
ssrRewriteStackTrace
in a try/catch and returning the stack as is if it throws.Again i understand this is a small enough edge case but i tought that given that sometimes
ssrRewriteStackTrace
can throw would still be a good addition.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.